Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent duplicate kernel startup #876

Merged
merged 3 commits into from
Jun 20, 2017
Merged

Conversation

BenRussert
Copy link
Member

Since we rely on store.kernel to indicate if a new kernel should be started store needs to be aware af startup pretty much immediately to avoid duplicate kernels. See #870 for a wordier explanation.

The idea here is to somehow fail silently instead of creating dup kernels that may not shut down properly. I would be grateful for feedback!

To Duplicate:

  • start a new kernel by pressing ctrl-enter on 2 or more lines quickly

example

@@ -10,7 +10,8 @@ import type Kernel from "./../kernel";

class Store {
subscriptions = new CompositeDisposable();
@observable runningKernels = new Map();
@observable startingKernels: Map<string, boolean> = new Map();
@observable runningKernels: Map<string, Kernel> = new Map();
@observable editor = atom.workspace.getActiveTextEditor();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally we could do this without even adding startingKernels, but we would have to somehow give store the new kernel before the kernel is actually ready. I suppose we could promisify or something, but am eager to see what you all think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having a promise or to just give store a new kernel even if it's not fully connected yet. But this likely requires some refactorings that will be a lot easier once #858 is closed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I'll use the phrase "we can iterate" here 😄

@BenRussert
Copy link
Member Author

This is ready for review!

There haven't been any comments on this or #876, what do you think?

@nikitakit I remembered that you had mentioned this issue in #780, do you have any feedback?

Copy link
Member

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgbkrk What do you think should we go with this?

It's not the most elegant solution but it works 😉

@@ -10,7 +10,8 @@ import type Kernel from "./../kernel";

class Store {
subscriptions = new CompositeDisposable();
@observable runningKernels = new Map();
@observable startingKernels: Map<string, boolean> = new Map();
@observable runningKernels: Map<string, Kernel> = new Map();
@observable editor = atom.workspace.getActiveTextEditor();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having a promise or to just give store a new kernel even if it's not fully connected yet. But this likely requires some refactorings that will be a lot easier once #858 is closed.

Copy link
Member

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with it 🚀

@lgeiger lgeiger merged commit 060760d into nteract:master Jun 20, 2017
@rgbkrk
Copy link
Member

rgbkrk commented Jun 20, 2017

🆒

@BenRussert BenRussert deleted the fix-kernelstart branch June 20, 2017 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants